-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Several JSX fixes (addresses #73) #123
Conversation
@@ -0,0 +1 @@ | |||
<BaseForm url="/auth/google" method="GET" colour="blue" size="large" submitLabel="Sign in with Google"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you want to move these under a new tests/jsx
directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests situation is a little weird, but I'm hesitant to touch much outside of the prettier
directory because all of those tests are Flow's test suite. In the future we should be able to remove our version and copy in the latest Flow tests.
colour=\"blue\" | ||
size=\"large\" | ||
submitLabel=\"Sign in with Google\" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most conventions I've seen is to put the >
in the same line as the last attribute. This is likely going to be a bit trickier to implement but I think that it's important.
Oh, that's actually easier. I did it this way on purpose as it conforms
with the Airbnb style guide, which is what I use. Happy to put it on the
trailing line if that's preferred. Might warrant some discussion though,
perhaps in #73
…On Wed, Jan 11, 2017, 10:59 Christopher Chedeau ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/prettier/__snapshots__/jsfmt.spec.js.snap
<#123 (review)>
:
> +/>;
+"
+`;
+
+exports[`test jsx_long_name_open.js 1`] = `
+"<BaseForm url=\"/auth/google\" method=\"GET\" colour=\"blue\" size=\"large\" submitLabel=\"Sign in with Google\">
+ hello
+</BaseForm>
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+<BaseForm
+ url=\"/auth/google\"
+ method=\"GET\"
+ colour=\"blue\"
+ size=\"large\"
+ submitLabel=\"Sign in with Google\"
+>
Most conventions I've seen is to put the > in the same line as the last
attribute. This is likely going to be a bit trickier to implement but I
think that it's important.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#123 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAq_LtaaC3HFNvhEHEDH-GibQId5_HYNks5rRSakgaJpZM4Lg65c>
.
|
The advantage of the Airbnb style is atomicity - similar to the argument in
favor of trailing commas. Better git diff's etc.
…On Wed, Jan 11, 2017, 11:02 Alex Rattray ***@***.***> wrote:
Oh, that's actually easier. I did it this way on purpose as it conforms
with the Airbnb style guide, which is what I use. Happy to put it on the
trailing line if that's preferred. Might warrant some discussion though,
perhaps in #73
On Wed, Jan 11, 2017, 10:59 Christopher Chedeau ***@***.***>
wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/prettier/__snapshots__/jsfmt.spec.js.snap
<#123 (review)>
:
> +/>;
+"
+`;
+
+exports[`test jsx_long_name_open.js 1`] = `
+"<BaseForm url=\"/auth/google\" method=\"GET\" colour=\"blue\" size=\"large\" submitLabel=\"Sign in with Google\">
+ hello
+</BaseForm>
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+<BaseForm
+ url=\"/auth/google\"
+ method=\"GET\"
+ colour=\"blue\"
+ size=\"large\"
+ submitLabel=\"Sign in with Google\"
+>
Most conventions I've seen is to put the > in the same line as the last
attribute. This is likely going to be a bit trickier to implement but I
think that it's important.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#123 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAq_LtaaC3HFNvhEHEDH-GibQId5_HYNks5rRSakgaJpZM4Lg65c>
.
|
Yeah, probably warrants some discussions. I didn't know that Airbnb recommended this. On the React docs we use the one I suggested and across our codebase at Facebook as well. |
TBH I don't understand why we do this at Facebook. Putting it on the new line means less friction when adding/removing attributes, and is more diff-friendly. Also easier to read when there is a bunch of attributes and children. |
Airbnb doesn't offer any justification, nor is there any in the eslint rule docs. Would it be easy to find out why FB does this? Would it be possible to change FB's internal guide (so that |
I agree, see this example: <button
color={this.props.color}
onClick={() => this.setState(state => ({count: state.count + 1}))}>
Count: {this.state.count}
</button> You don't really see where the opening tag ends. It looks like |
I don't think that there's a good logical argument for Now, this is a pretty visible change and there's already going to be a lot of friction for integrating |
It'd be an easy option to make, but need to decide if we should have that option or not. If that's the only thing that would stop Facebook from adopting this it's tempting to make it an option, but I also want to be fair to other users who want other options. @vjeux What do you think about putting |
Let's do that. There is a bunch more blockers until we can actually use it. When that list is down to something where we can actually test it, we'll see how people react. |
Awesome!
Is this okay to merge or is there anything I can improve here?
…On Wed, Jan 11, 2017, 13:09 Christopher Chedeau ***@***.***> wrote:
Let's do that. There is a bunch more blockers until we can actually use
it. When that list is down to something where we can actually test it,
we'll see how people react.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#123 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAq_LjVxWpZLlfrNkXPd7tVn6cPPy43Tks5rRUT1gaJpZM4Lg65c>
.
|
I added a second commit to this PR fixing parens. Let me know if I should split that out to a separate PR. |
submitLabel=\"Sign in with Google\" | ||
/>; | ||
|
||
long_open = <BaseForm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, since you add () around arrow functions, should you also do it here for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly...
In this case, I'm only using variables so that one file can contain multiple JSX statements, which otherwise can't be siblings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would seem to conform to AirBnB's guide so sure why not!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I should probably refactor this (and any others like the corresponding code in ReturnStatement
) within JSXElement
itself... might be tricky...
I really like your approach of going through the Airbnb style-guide as a reference when in doubt :) |
b6c3b60
to
8f652af
Compare
Also added a fix for #73 (comment) , though using |
@rattrayalex This will conflict with #126, any chance you could remove the commits that do the escaping? I'm not sure I want to force the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
Yep can do! |
indent(options.tabWidth, concat([ line, path.call(print, "body") ])), | ||
])); | ||
} | ||
return conditionalGroup(groups); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer avoiding conditionalGroup
in this special case which makes this code path a little clearer:
const body = path.call(print, "body");
const collapsed = concat([ concat(parts), " ", body ]);
if (n.body.type === "JSXElement") {
return group(collapsed);
}
return conditionalGroup([
collapsed,
groups.push(concat([
concat(parts),
indent(options.tabWidth, concat([ line, body ])),
]));
])
Also note that I lifted path.call(print, "body")
out which is very important. I wrote that code so it's my fault, but when using conditionalGroup
you should never print the same node twice because it makes the intermediate representation explode in size (we want to share sub-trees).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, will do!
hah, I'd originally wrote it with body
separated as you have now, and changed it back. Good to hear of the tradeoffs
")", | ||
])); | ||
|
||
return conditionalGroup(groups); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need conditionalGroup
here. Use conditionalGroup
only as a last resort :) (I plan on writing more documentation about all this)
conditionalGroup
is only needed when you really need manual control over how things are displayed when they don't fit. The majority of use cases fits into these 2 categories though:
- You want to break something across lines and indent if it doesn't fit (this assumes nothing inside of it has "hard" lines, meaning all contents try to fit on the same line)
- You want to break something across lines if any of its children are broken across lines (they have "hard" lines)
group
solves #1 easily: use indent
and line
and the printer, when the group doesn't fit, will output newlines where line
was used and it will be indented an extra level.
I made multilineGroup
for #2 because it's a very common use case. It will see if any of its contents have hard lines, and if they do, the group will break. Hard lines are lines that emit a newline no matter what. The reason we don't have to worry about normal breaking is because this group is always larger than it's contents -- if anything inside of it naturally breaks, this group will already have broken as well.
There are some rare cases where you want to let something inside of you break, but not break the group, and control indentation rules in each specific case. That's what conditionalGroup
is for, but we should use it rarely as it has a performance cost.
There's one last character in this cast: ifBreak
. This is a very simple construct that does what it says: if the group break, include this content. You can say ifBreak(",")
and it will only include the comma if the group has broken. The solves some very simple use cases.
Will all that said, I think this should work:
multilineGroup(concat([
ifBreak("("),
indent(options.tabWidth, concat([ softline, elem ])),
softline,
ifBreak(")"),
]))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's extremely helpful! Also sounds like a great cleanup. Thanks!
e033326
to
bc08eb4
Compare
|
||
const render5 = ({ styles }) => <div>Keep it on one line.</div>; | ||
|
||
const notJSX = (aaaaaaaaaaaaaaaaa, bbbbbbbbbbb) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is glorious
@@ -0,0 +1 @@ | |||
blah |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only thing left to do, right? Once you remove this, is this ready to be merged? Also check the test failures (slow to respond right now, will focus more later today)
@rattrayalex When you rebase locally, you should be able to just |
Sorry, I hadn't run (children need to be broken up if the open element is multiline) |
Sure, feel free to add any small fixes to this PR. I wanted to do another pass at JSX support and improve many things at once, and this is basically that! Thanks. |
Awesome! have a few more small fixes in the pipeline, should be done by EOD or tomorrow. |
const firstChild = children[0]; | ||
const lastChild = util.getLast(children); | ||
const beginBreak = firstChild && firstChild.type === "line"; | ||
const endBreak = lastChild && lastChild.type === "line"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jlongster my beginBreak
/ endBreak
logic here felt a bit awkward – I'm adding a softline
unless there's already a line. Is there a cleaner way to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rattrayalex Sorry I didn't review this until now.
I'm not entirely sure of the logic you're going for here, but is util.newlineExistsBefore
helpful? If you're trying to emulate the breaking behavior in the original source, that and util.newlineExistsAfter
allows you to peek into the original source and see if a node is on a newline or not. Here's how you use it:
util.newlineExistsBefore(text, util.locStart(node))
What this does is it scans backward until it hits either a newline or a non-whitespace character. If it hits a newline, it reports true
, otherwise it's false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely helpful to know about, though in this case I think the lines could be inserted by prettier
, not just from the original source. So I think it wouldn't be adequate in this case.
But it would be very helpful when deciding whether {' '}
needs to be inserted, if that's a route we go.
attr4 | ||
> | ||
ddd d dd d d dddd dddd <strong>hello</strong> | ||
</div><strong>hello</strong> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, I'm trying to fix this now (bump siblings to own line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't figure out a clean solution to this, so leaving for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can do that. We would be adding significant whitespace, which changes the program. That's a limitation because of how JSX works; we aren't as free to move things around as other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it turns out, adding newlines between JSX nodes doesn't add whitespace between the nodes to the compiled HTML. So that would be a safe change to make in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would have to be a newline & a bunch of spaces for indentation. How could that not add whitespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSX isn't HTML, the elements compile to function calls:
React.createElement('div', {}, [
React.createElement('span', {}, ['hello world']),
React.createElement('strong', {}, ['I am a sibling'])
])
^^ there's no whitespace between the <span />
and the <strong />
above. That would render to:
<div><span>hello world</span><strong>I am a sibling</strong></div>
const render7 = () => ( | ||
<div> | ||
<span /> | ||
<span>Break each elem onto its own line.</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty iffy on this. There don't seem to be any eslint rules about preventing multiple JSX elements on one line...
I think I'll cut this, even if it means having this on the same line:
</div><span>should really be next line</span>
though if anyone can figure out a solution to that case, I'm all ears
Hmm, the other challenges I had hoped to tackle look a bit too challenging for my current knowledge of the tools at hand. (Actually, I suspect they may not be possible with the current tools at hand). Specifically:
Would appreciate guidance, though for now my preference would be to merge this PR. Perhaps more skilled hands than mine could take up the remainder of this work thereafter. |
Thanks a lot for working on this. It's definitely fine to do other things as followup. I'll look at this soon. |
FYI, we recently rewrote the JSX documentation to explain how whitespace works:
<div>Hello World</div>
<div>
Hello World
</div>
<div>
Hello
World
</div>
<div>
Hello World
</div> https://facebook.github.io/react/docs/jsx-in-depth.html#children-in-jsx |
@vjeux @rattrayalex oh, shows how much I know about JSX 😆 I'm just now starting to use it, not that I can consistently format it. I'm not sure we want to be aggressive about bumping tags to newlines. JSX seems like something that the programmer lays out specifically, so we should be more lenient about respecting JSX. There was someone else talking about not doing this specifically (can't remember where) so that if you had a big paragraph you could have |
@rattrayalex Can you look at my one comment and see if those utility functions are what you're after? Once we get that comment resolved, I will merge this. Thanks so much for all the hard work! |
Alright this has been sitting for too long. I'm eager to get it in and make a release, so I'll go ahead and merge it. Thanks for the excellent work @rattrayalex. This will make prettier usuable for JSX folks! I'm going to follow-up with the comments I made and possibly make some PRs myself. But if everything mostly checks out, I'll push a new release. |
I wasn't sure where to add the new test cases, so I just threw them as separate files in the
prettier
dir. Happy to move/rename etc as desired.